Skip to content

fix: align nudge summary token counts with tokenizer - #91

Open
Beatrice0377 wants to merge 4 commits into
ranxianglei:masterfrom
Beatrice0377:2026-08-19_nudge-summary-tokens
Open

fix: align nudge summary token counts with tokenizer#91
Beatrice0377 wants to merge 4 commits into
ranxianglei:masterfrom
Beatrice0377:2026-08-19_nudge-summary-tokens

Conversation

@Beatrice0377

Copy link
Copy Markdown

Problem

Tier-distillation nudges display each target block's summary token count, but the renderer estimated that value with summary.length / 4.

The decision layer already uses the injected countTokens function for the same summaries. This makes the displayed compression payoff diverge from the value used for tier arbitration, especially for CJK text and custom tokenizers.

Root Cause

pendingByTier had access to the active countTokens, but NudgeDecision only carried the target CompressionBlocks.

The per-block token counts were therefore lost at the decision boundary, forcing renderNudgeText to estimate them again with length / 4.

Fix

  • Precompute per-target-block summary token counts in the decision layer using the active injected countTokens.
  • Derive pendingT2 / pendingT3 from those same precomputed values so arbitration and rendering share one token-counting source.
  • Carry the derived values on NudgeDecision as tierTargetBlockStats.
  • Make the renderer consume those values instead of re-tokenizing normal kernel-produced decisions.
  • Preserve the previous length / 4 behavior as a compatibility fallback for manually constructed legacy decisions that do not provide stats.

Scope / Non-goals

  • CompressionBlock is unchanged; derived token counts are not persisted into runtime state.
  • renderNudgeText(decision, prompts?) keeps its existing signature.
  • No dependency, version, lockfile, workflow, or documentation changes.
  • No unrelated refactors.

Tests

Added regression coverage for:

  • CJK summaries using the default CJK-aware tokenizer.
  • A custom injected tokenizer that differs from both length / 4 and defaultCountTokens.
  • Renderer use of precomputed per-block token counts.
  • Legacy decisions without stats.
  • ASCII output compatibility.

Validation:

  • Targeted tests: 47/47 passed.
  • Full suite: 390/390 passed.
  • npm run typecheck: passed.
  • npm run build: passed.
  • git diff --check: passed.

Compatibility

The new NudgeDecision.tierTargetBlockStats field is optional.

Existing manually constructed decisions without the field continue to use the previous renderer estimate, while normal kernel-produced tier-2/tier-3 decisions provide tokenizer-consistent stats.

Limitations

Legacy/manual decisions that omit tierTargetBlockStats intentionally retain the previous fallback estimate for backward compatibility.

Fixes #45

@ranxianglei ranxianglei left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review: ✅ Approve — 2 independent agent reviews, both approve. Nits below are optional, none blocking.

Verified locally on the PR branch (branch current with master incl. v0.0.31):

  • npm run typecheck
  • npm test → 396/396 ✅ (master 391 + 5 new; PR body's "390/390" just reflects older master at authoring time — count is consistent)
  • npm run build
  • CI green (pr-validation, test 22/24); mergeable CLEAN; both issue #45 named regression tests pass in isolation
  • Caller audit: renderNudgeText/formatTierTargetBlocks have a single render path in src (exported via src/index.ts:35); no other estimator call sites remain

Design — implements #45's preferred option exactly:

  • Precompute once in the decision layer (pendingByTier, src/compress.ts) with the active injected countTokens; renderer stays dependency-free and consumes plain data (src/nudge-text.ts:39-46). Displayed number can no longer diverge from the arbitration number.
  • pendingT2/pendingT3 now reduce over the same per-block stats (src/compress.ts:961-967) — gate inputs are value-identical to master; no behavior change to arbitration.
  • TierTargetBlockStat not persisted on CompressionBlock — correct, since countTokens is host-injected and a stored value would go stale (rationale documented in src/types.ts).
  • Renderer joins stats by blockId via Map, never by index — ordering drift cannot misattribute numbers. blockIds are unique by construction (monotonic counter, src/state.ts:16-23), so no duplicate-key risk. ?? correctly preserves a legitimate 0 token count.
  • Legacy length / 4 retained only as fallback for hand-built decisions without stats — backward compatible, and covered by a dedicated compat test.

Tests — good coverage: custom tokenizer that differs from both length/4 and defaultCountTokens (nice fake-fix guard), CJK-aware default tokenizer end-to-end (decision stats → pendingT2 aggregation → rendered text), legacy fallback, and ASCII-identity when stats equal the legacy estimate.

Nits (optional)

  1. No tier-3 stat assertion (code is symmetric with tier-2 — low risk).
  2. No test for a partial-stats array (one block missing from the Map → falls back per-block).
  3. Test-only ! non-null assertions — pre-existing pattern in this suite, fine as-is.

Fixes #45 as claimed. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

nudge text shows summary tokens estimated via length/4, inconsistent with the CJK-aware tokenizer

2 participants